fix(oauth2): idempotent OAuth2 completion and resilient listener#40292
fix(oauth2): idempotent OAuth2 completion and resilient listener#40292betodealmeida wants to merge 3 commits into
Conversation
betodealmeida
changed the title
dosubot
Bot
added
authentication:sso
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40292 +/- ##
==========================================
- Coverage 64.13% 64.08% -0.06%
==========================================
Files 2591 2592 +1
Lines 138367 138809 +442
Branches 32094 32182 +88
==========================================
+ Hits 88747 88960 +213
- Misses 48090 48317 +227
- Partials 1530 1532 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments. |
sadpandajoe
added
the
review:checkpoint
|
Huh, I fixed and pushed, but my commit is not showing up here. |
|
Had to push an empty commit to make it work! :D |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| handled = true; | ||
| const { | ||
| source: src, | ||
| query: q, | ||
| chartId: cId, | ||
| chartList: cList, | ||
| dashboardId: dId, | ||
| } = latestStateRef.current; | ||
| if (src === 'sqllab' && q) { | ||
| dispatch(reRunQuery(q)); | ||
| } else if (src === 'explore' && cId) { | ||
| dispatch(triggerQuery(true, cId)); | ||
| } else if (src === 'dashboard') { | ||
| dispatch(onRefresh(cList.map(Number), true, 0, dId)); |
There was a problem hiding this comment.
Suggestion: The deduplication flag is set before confirming any action was actually dispatched. If the first matching signal arrives when the required state is not ready (for example source === 'sqllab' but query is still null), the handler marks the event as handled and drops later duplicate/fallback signals that could have succeeded, causing the OAuth completion to be missed. Set the handled flag only after a dispatch path runs successfully. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ SQL Lab queries may not auto re-run after OAuth.
- ⚠️ Explore chart reload can be skipped in rare timing windows.
- ⚠️ Dashboard auto-refresh after OAuth may silently fail.Steps of Reproduction ✅
1. In an OAuth2-enabled database connection, a query path wrapped in `check_for_oauth2()`
at `superset/utils/oauth2.py:311-320` raises an `OAuth2RedirectError`
(`superset/exceptions.py:341-367`), which is surfaced to the frontend as an
`OAUTH2_REDIRECT` error rendered by `OAuth2RedirectMessage` at
`superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:64-98`.
2. When `OAuth2RedirectMessage` mounts with `source="sqllab"`, it derives `query` from
Redux via `queries`, `queryEditors`, and `tabHistory` selectors at
`OAuth2RedirectMessage.tsx:71-86`, and stores that state in `latestStateRef.current` at
`OAuth2RedirectMessage.tsx:105-118`; if for any reason `qe?.latestQueryId` is unset or
`queries[qe.latestQueryId]` is missing, `query` is `null`.
3. After the user completes OAuth2 in the popup, the backend callback
`DatabasesRestApi.oauth2_authorized` at `superset/databases/api.py:10-21` renders
`superset/templates/superset/oauth2.html`, whose inline script posts the `{ tabId }`
message on the `'oauth'` `BroadcastChannel` and emits a localStorage change at
`superset/templates/superset/oauth2.html:27-44`.
4. On the original tab, the `useEffect` in `OAuth2RedirectMessage` registers both a
`BroadcastChannel` listener and a `storage` listener (lines `145-172`) that call
`handleOAuthComplete` (lines `123-143`); on the first matching event `handleOAuthComplete`
sets `handled = true` at `128` before reading `latestStateRef.current` (lines `129-135`,
where `q` may still be `null`), so the `if (src === 'sqllab' && q)` branch at `136-137` is
skipped and no dispatch occurs, and because `handled` is already `true`, any later
duplicate/fallback signal is ignored by the early guard `if (tabId !== extra.tab_id ||
handled)` at `125-127`, causing the OAuth completion re-run to be permanently dropped.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx
**Line:** 128:141
**Comment:**
*Incorrect Condition Logic: The deduplication flag is set before confirming any action was actually dispatched. If the first matching signal arrives when the required state is not ready (for example `source === 'sqllab'` but `query` is still null), the handler marks the event as handled and drops later duplicate/fallback signals that could have succeeded, causing the OAuth completion to be missed. Set the handled flag only after a dispatch path runs successfully.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Yes — this comment is relevant.
The current logic can mark the event as handled before confirming that any refresh/re-run actually happened. That means an early signal arriving before query, chartId, or dashboard state is ready could permanently block the later fallback signal.
Recommended fix
Move handled = true so it is set only after a dispatch path succeeds.
For example:
const handleOAuthComplete = (tabId?: string) => {
if (tabId !== extra.tab_id || handled) {
return;
}
const {
source: src,
query: q,
chartId: cId,
chartList: cList,
dashboardId: dId,
} = latestStateRef.current;
if (src === 'sqllab' && q) {
handled = true;
dispatch(reRunQuery(q));
} else if (src === 'explore' && cId) {
handled = true;
dispatch(triggerQuery(true, cId));
} else if (src === 'dashboard') {
handled = true;
dispatch(onRefresh(cList.map(Number), true, 0, dId));
}
};This preserves the deduplication behavior, but avoids dropping a later valid signal when the first one arrives too early.
If you want, I can also review the remaining comments on this PR and help fix those too.
Code Review Agent Run #dce8baActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Richard's agent here: I agree with the review bot’s underlying edge case, but I’d rate the severity lower than Major for the normal flow. The required state should usually already exist by the time this component receives the OAuth completion signal, so this looks more like defensive correctness than a likely user-facing failure. That said, the fix is small and makes the dedupe semantics more precise: set |
4 participants
SUMMARY
The OAuth2 callback page always emitted both
BroadcastChannelandlocalStoragesignals whenBroadcastChannelwas supported, so a single completion re-triggeredreRunQuery/triggerQuery/onRefreshtwice in the original tab. This PR makes the sender's storage path a true fallback (only whenBroadcastChannelis unavailable or its construction throws), and guard the receiver against duplicate dispatches as a safety net.Additionally, it wraps channel/storage operations in
try/catchso restricted contexts can't abort the flow, and stabilize the listener's effect by reading volatile selector values (chartList,query, etc.) from arefsochartListchanges don't churn the channel and open a race window.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION